pull: Error if gpg=true and summary is 404, add more tests

I did a quick audit pass through the pull code.  What I focused on the
most is the case where `gpg-verify-summary=true`, and in particular
where `gpg-verify=false` too.  This should be a valid and secure
configuration.

The primary change here is to error out very quickly if either
`summary` or `summary.sig` are 404.  Previously, we'd only error out
if we were processing deltas.

Expand the existing test case to cover this, plus invalid summary and
invalid sig.  (The test case was failing with current git master too).

diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index 96c3a188aa54e08bf5bfc73a18edc69f8d93aab9..29b72bf4f5c21f638e4be5e432151518d69c2c40 100644 (file)
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -1318,7 +1318,10 @@ request_static_delta_superblock_sync (OtPullData  *pull_data,
         delta = g_strconcat (from_revision ? from_revision : "", from_revision ? "-" : "", to_revision, NULL);
         summary_csum = g_hash_table_lookup (pull_data->summary_deltas_checksums, delta);
 
-
+        /* At this point we've GPG verified the data, so in theory
+         * could trust that they provided the right data, but let's
+         * make this a hard error.
+         */
         if (pull_data->gpg_verify_summary && !summary_csum)
           {
             g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
@@ -1818,27 +1821,41 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
         soup_uri_free (uri);
      }
 
-    if (bytes_summary)
+    if (!bytes_summary && pull_data->gpg_verify_summary)
       {
-        g_autoptr(GVariant) sig_variant = NULL;
-        glnx_unref_object OstreeGpgVerifyResult *result = NULL;
-        pull_data->summary_data = g_bytes_ref (bytes_summary);
-        pull_data->summary = g_variant_new_from_bytes (OSTREE_SUMMARY_GVARIANT_FORMAT, bytes_summary, FALSE);
+        g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                     "GPG verification enabled, but no summary found (use gpg-verify-summary=false in remote config to disable)");
+        goto out;
+      }
 
+    if (bytes_summary)
+      {
         uri = suburi_new (pull_data->base_uri, "summary.sig", NULL);
         if (!fetch_uri_contents_membuf_sync (pull_data, uri, FALSE, TRUE,
                                              &bytes_sig, cancellable, error))
           goto out;
         soup_uri_free (uri);
+      }
 
-        if (bytes_sig)
-          pull_data->summary_data_sig = g_bytes_ref (bytes_sig);
-        else
-          {
-            g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
-                         "GPG verification enabled, but no summary signatures found (use gpg-verify-summary=false in remote config to disable)");
-            goto out;
-          }
+    if (!bytes_sig && pull_data->gpg_verify_summary)
+      {
+        g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                     "GPG verification enabled, but no summary.sig found (use gpg-verify-summary=false in remote config to disable)");
+        goto out;
+      }
+
+    if (bytes_summary)
+      {
+        pull_data->summary_data = g_bytes_ref (bytes_summary);
+        pull_data->summary = g_variant_new_from_bytes (OSTREE_SUMMARY_GVARIANT_FORMAT, bytes_summary, FALSE);
+      }
+
+    if (bytes_summary && bytes_sig)
+      {
+        g_autoptr(GVariant) sig_variant = NULL;
+        glnx_unref_object OstreeGpgVerifyResult *result = NULL;
+
+        pull_data->summary_data_sig = g_bytes_ref (bytes_sig);
 
         sig_variant = g_variant_new_from_bytes (OSTREE_SUMMARY_SIG_GVARIANT_FORMAT, bytes_sig, FALSE);
         result = _ostree_repo_gpg_verify_with_metadata (self,

diff --git a/tests/test-pull-summary-sigs.sh b/tests/test-pull-summary-sigs.sh
index a57a9eb1d2f789ad87a59c89b157df377d1b3662..b0acf46088fdb82ed93749cc1a3cac594f0d5690 100644 (file)
--- a/tests/test-pull-summary-sigs.sh
+++ b/tests/test-pull-summary-sigs.sh
@@ -60,22 +60,46 @@ cd $prev_dir
 
 ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
+repo_reinit () {
+  cd ${test_tmpdir}
+  rm -rf repo
+  mkdir repo
+  ${OSTREE} --repo=repo init --mode=archive-z2
+  ${OSTREE} --repo=repo remote add --set=gpg-verify-summary=true origin $(cat httpd-address)/ostree/gnomerepo
+}
+
 cd ${test_tmpdir}
-rm -rf repo
-mkdir repo
-${OSTREE} --repo=repo init --mode=archive-z2
-${OSTREE} --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo
+repo_reinit
 ${OSTREE} --repo=repo pull origin main
 echo "ok pull with signed summary"
 
+cd ${test_tmpdir}
+repo_reinit
+mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.good}
+echo invalid > ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig
+if ${OSTREE} --repo=repo pull origin main 2>err.txt; then
+    assert_not_reached "Successful pull with invalid GPG sig"
+fi
+assert_file_has_content err.txt "no signatures found"
+mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.good,}
+echo "ok pull with invalid summary gpg signature fails"
+
+cd ${test_tmpdir}
+repo_reinit
+cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{,.good}
+# Some leading garbage
+(echo invalid && cat ${test_tmpdir}/ostree-srv/gnomerepo/summary) > summary.bad.tmp && mv summary.bad.tmp ${test_tmpdir}/ostree-srv/gnomerepo/summary
+if ${OSTREE} --repo=repo pull origin main; then
+    assert_not_reached "Successful pull with invalid summary"
+fi
+mv ${test_tmpdir}/ostree-srv/gnomerepo/summary{.good,}
+echo "ok pull with invalid summary (leading garbage) fails"
+
 # Generate a delta
 ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate --empty main
 ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
 cd ${test_tmpdir}
-rm -rf repo
-mkdir repo
-${OSTREE} --repo=repo init --mode=archive-z2
-${OSTREE} --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo
+repo_reinit
 ${OSTREE} --repo=repo pull origin main
 echo "ok pull delta with signed summary"